Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

feat: add regtest to btcwallet #975

Merged
merged 5 commits into from
Feb 21, 2025
Merged

feat: add regtest to btcwallet #975

merged 5 commits into from
Feb 21, 2025

Conversation

Abdulkbk
Copy link
Contributor

@Abdulkbk Abdulkbk commented Feb 11, 2025 •
edited
Loading

closes #955

Following up on the comment, In this PR we add regtest support to btcwallet.

A regtest environment can benefit users who want to test their applications or programs. Specifically, I am working on a project to add BTCD support in Polar, which depends on this, as all other node implementations operate on regtest. This becomes a blocker as highlighted in this comment:

The second blocker is that btcwallet does not support regtest currently, meanwhile all the other node implementations in Polar use regtest. I think this is the real challenge.

Steps to test

  • Start BTCD in regtest
btcd --regtest
  • Initialize btcwallet on regtest
btcwallet --regtest --create
  • Finally start btcwallet
btcwallet --regtest

You can access btcwallet using the default port, which is set as 18332.

@Abdulkbk
Copy link
Contributor Author

Could you help approve CI @yyforyongyu 🙏

@yyforyongyu
Copy link
Collaborator

Approved CI run.

@Abdulkbk
Copy link
Contributor Author

The linter is throwing an error about tagalign which I don't quite get.

Error: cmd/sweepaccount/main.go:46:44: tag is not aligned, should be: description:"Use the regression bitcoin network"          long:"regtest" (tagalign)
	RegressionNet         bool                `long:"regtest" description:"Use the regression bitcoin network"`

Interestingly, the other lines follow the same format, yet the linter only complains about this. I wonder if this has any connection with this comment?. I would look into it deeper and see what I will find.

@guggero
Copy link
Collaborator

guggero commented Feb 13, 2025

The linter is throwing an error about tagalign which I don't quite get.

Error: cmd/sweepaccount/main.go:46:44: tag is not aligned, should be: description:"Use the regression bitcoin network"          long:"regtest" (tagalign)
	RegressionNet         bool                `long:"regtest" description:"Use the regression bitcoin network"`

Interestingly, the other lines follow the same format, yet the linter only complains about this. I wonder if this has any connection with this comment?. I would look into it deeper and see what I will find.

Hmm, I've never seen that linter error before. Only seems to mean the formatting of the tags, which looks okay to me.
Feel free to add tagalign to the list of disabled linters in .golangci.yml.

@Abdulkbk Abdulkbk force-pushed the regtest-support branch 3 times, most recently from 7d97082 to be21cd7 Compare February 13, 2025 17:17
@Abdulkbk
Copy link
Contributor Author

Hmm, I've never seen that linter error before. Only seems to mean the formatting of the tags, which looks okay to me. Feel free to add tagalign to the list of disabled linters in .golangci.yml.

I disabled tagalign and resolved the issue. I then encountered some linting issues with characters exceeding the max (120):

config.go:100: the line is 205 characters long, which exceeds the maximum of 120 characters. (lll)
        LegacyRPCListeners     []string                `long:"rpclisten" description:"Listen for legacy RPC connections on this interface/port (default port: 8332, testnet: 18332, simnet: 18554, regtest: 18332)"`

Fixed that by adding //nolint:lll just above the config struct definition, just like in LND

@kelis-cpu
Copy link

thx man! you are my hero!!!

yyforyongyu
yyforyongyu reviewed Feb 17, 2025
View reviewed changes
Copy link
Collaborator

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! I wonder if there's a way to test it?

@Abdulkbk
main: add regtest to config
7bb88e5
@Abdulkbk
main+netparams: add net params for regtest
5b6a990
@Abdulkbk
main: add regression net to sweepaccount
e998089
@Abdulkbk
keystore: use regtest related settings
3e2c270 
In this commit, we add checks to set the right settings when the
user is on regtest.
@Abdulkbk
lint: fix issues with lint
676c400 
In this commit we disable tagalign and turn off lll lint
on config struct
@Abdulkbk
Copy link
Contributor Author

Looking good! I wonder if there's a way to test it?

Are you specifically referring to writing tests for regtest?

yyforyongyu
yyforyongyu approved these changes Feb 18, 2025
View reviewed changes
Copy link
Collaborator

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you specifically referring to writing tests for regtest?

More like testing the config validations are applied, but it's minor. As for the regtest, I think it's missing an integration test framework (sth like itest used in lnd), which I will write about it.

@Abdulkbk
Copy link
Contributor Author

More like testing the config validations are applied, but it's minor. As for the regtest, I think it's missing an integration test framework (sth like itest used in lnd), which I will write about it.

Alright, I'm looking forward to what you'll write then.

Also what next steps would you recommend I take to move this PR forward?

@yyforyongyu
Copy link
Collaborator

Also what next steps would you recommend I take to move this PR forward?

Think we just need another reviewer🤓

@guggero guggero self-requested a review February 20, 2025 16:40
guggero
guggero approved these changes Feb 21, 2025
View reviewed changes
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK, LGTM 🎉

@guggero guggero merged commit 311812f into btcsuite:master Feb 21, 2025
3 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@guggero guggero guggero approved these changes

@yyforyongyu yyforyongyu yyforyongyu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Why regtest network is absent in btcwallet?
4 participants
@Abdulkbk @yyforyongyu @guggero @kelis-cpu